EDA blueprints #4771
EDA blueprints #4771
Conversation
|
Hello, this PR is about the files in the examples/eda folder. It adds two new blueprints and a readme.md. All the other changes are from #4583 and not subject of this PR. I am going to rebase to develop as soon an #4583 is merged. I just want to get the review process started on the two blueprints and the readme file. Kudos to @thomasnyc for providing the H4D compute part of these blueprints. |
okrause
force-pushed
the
eda-blueprints
branch
from
033d96a to
5dace5b
Compare
okrause
force-pushed
the
eda-blueprints
branch
2 times, most recently
from
0b5fe14 to
cb4e18d
Compare
|
@sarthakag Can you please have a look? Should these blueprints be added as core or community? I am open to both. Do we need tests? |
|
@sarthakag BTW: We cannot add an test for ./examples/eda/eda-hybrid-cloud.yaml, as this required the user to provide existing volumes. Apart from that, it is basically the same as the eda-all-cloud blueprint anyways. |
|
Hi @okrause, can you please rebase ? Had a quick look over the PR, left a few comments. Will look more into the integration tests and get back to you in a week's time. Thanks! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new blueprints for Electronic Design Automation (EDA) workloads, along with comprehensive documentation. The changes are well-structured and provide valuable examples for users. My review focuses on improving the clarity and correctness of the documentation and enhancing the configuration of the new blueprint files. I've identified several typos and inconsistencies in the README files that should be addressed. In the YAML blueprints, I've pointed out a hardcoded value that should use a variable for better flexibility and a security concern regarding overly permissive file permissions.
…imageUpdate Updated the instance_image.family in a3ultra-vm.yaml to use ubuntu-accelerator-2204-amd64-with-nvidia-570 instead of nvidia-550 for improved compatibility and performance.
…imageUpdate Updated the instance_image.family in a3ultra-vm.yaml to use ubuntu-accelerator-2204-amd64-with-nvidia-570 instead of nvidia-550 for improved compatibility and performance.
okrause
force-pushed
the
eda-blueprints
branch
from
eecfce0 to
be15d9f
Compare
|
@sarthakag Thank you for the review. The gemini review is also very helpful to find small gotchas which easily go unnoticed. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
bytetwin
left a comment
bytetwin
left a comment
There was a problem hiding this comment.
- Both the blueprints and its Readme should be in community/examples. Examples folder is for core blueprints that cluster toolkit team supports and will continue to maintain. Netapp volume is gcp supported product and is in core module as it can be used as filestore, these blueprints are for specific workload type which cluster toolkit team won't support.
- Please avoid adding images to the repo. Cluster toolkit is cloned from github by users as well as automated systems and repository size will impact the cloning speed.
@bytetwin Understood. Moved to community/examples/eda. Removed the pictures. Especially the hybrid use case benefit a lot from a picture, so I added a link to a blog where the interested user can find a picture of the concept. Pushed the changes. Thank you for your input! |
|
git history of this one is messy. For the merge I created clean PR #4931. Closing this one. |
5 participants
This PR adds two blueprints and a README.MD file to a new examples/eda folder.
Both blueprints use Google Cloud NetApp Volumes as the shared NFS storage for the compute cluster. The eda-all-on-cloud blueprint builds on top of #4583. Since that one is not merged, this PR looks pretty big, but it is only a folder, a readme and two blueprint files.
I open it as a draft PR for now. To make it ready for prime time, the following things need to happen: